tests/input_flows.py (1)

2824-2828: Prefer explicit method allow-list in recovery constructors.

Line 2824 and Line 2855 currently treat any non-Display value as the “non-word-count path”. That works for N4W1, but it can silently accept unsupported future values. Fail fast with an explicit allow-list.

Suggested hardening diff

@@
-        self.method = method
-        if method is messages.BackupMethod.Display:
+        self.method = method
+        if method not in (
+            messages.BackupMethod.Display,
+            messages.BackupMethod.N4W1,
+        ):
+            raise ValueError(f"Unsupported backup method: {method!r}")
+        if method is messages.BackupMethod.Display:
             self.word_count = len(shares[0].split(" "))
         else:
             self.word_count = None
@@
-        self.method = method
-        if self.method is messages.BackupMethod.Display:
+        self.method = method
+        if self.method not in (
+            messages.BackupMethod.Display,
+            messages.BackupMethod.N4W1,
+        ):
+            raise ValueError(f"Unsupported backup method: {self.method!r}")
+        if self.method is messages.BackupMethod.Display:
             self.word_count = len(shares[0].split(" "))
         else:
             self.word_count = None

Also applies to: 2855-2859

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/input_flows.py` around lines 2824 - 2828, Replace the broad "else"
handling that treats any non-Display BackupMethod as the non-word-count path
with an explicit allow-list: in the blocks that check "if method is
messages.BackupMethod.Display" (and the similar block around lines 2855-2859),
explicitly branch for each supported enum value (e.g., "elif method is
messages.BackupMethod.<SUPPORTED_METHOD>" for methods that should set word_count
= None) and add a final "else: raise ValueError(f'Unsupported BackupMethod:
{method}')" to fail fast on unknown future values so the constructors (where
word_count is assigned) only accept known BackupMethod members.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

core/src/apps/management/reset_device/layout.py (1)

237-243: Finish the missing N4W1 intro/safety screens.

intro() is still empty and backup() skips the warning shown by _DisplayBackup, so this path drops straight into writing secret shares to tags. I'd close those TODOs before enabling the flow outside demo builds.

Do you want me to sketch a minimal version that reuses the existing backup intro/warning layouts?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/apps/management/reset_device/layout.py` around lines 237 - 243, The
intro() and backup() stubs in ResetDevice.layout must present the same N4W1
intro and safety screens used elsewhere instead of being no-ops; implement
intro(self, num_of_words) to render the existing backup/introduction text/UI
(reuse the layout or components used by the prior backup flow), and update
backup(self, iter_shares) to invoke the existing _DisplayBackup
warning/confirmation sequence before proceeding to write shares (so the user
must acknowledge safety). Locate and call the same helper/layout functions or
components used by the current backup flow (e.g., the backup intro screen and
_DisplayBackup warning) inside ResetDevice.layout.intro and
ResetDevice.layout.backup, preserving the num_of_words and iter_shares
parameters.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

core/src/apps/management/recovery_device/layout.py (1)

283-291: ⚠️ Potential issue | 🟡 Minor

Don't warn on the intentional method=None path.

When USE_N4W1 is off, this still logs Unsupported backup method: None for the normal default flow. Limit the warning to explicit non-Display values.

Proposed fix

-        if method is not BackupMethod.Display and __debug__:
+        if method not in (None, BackupMethod.Display) and __debug__:
             from trezor import log

             log.warning(__name__, "Unsupported backup method: %s", method)

Based on learnings, method=None passed to choose_backup_method() is intentionally distinct from BackupMethod.Display, and debug-mode unsupported-method warnings should be suppressed for None as well as for BackupMethod.Display.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/apps/management/recovery_device/layout.py` around lines 283 - 291,
The warning in choose_handler currently triggers for method==None; change the
condition so the debug warning only runs when method is neither None nor
BackupMethod.Display: update the if to check "method is not None and method is
not BackupMethod.Display and __debug__" (keep the import of BackupMethod and
return of _DisplayHandler unchanged) so intentional None flows do not log an
unsupported-method warning.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.github/workflows/core.yml (1)

334-339: Don't silently drop UI checks on the N4W1 universal/noasan path.

For this matrix entry ACTIONS_DO_UI_TEST is still true, but the N4W1 branch switches to test_emu_multicore instead of test_emu_ui_multicore. That loses the --ui=test --ui-check-missing --do-master-diff coverage the standard path gets for the same job shape.

Possible fix

       - name: Run device tests (N4W1)
         if: ${{ matrix.n4w1 }}  # TODO: test N4W1 UI fixtures as well
-        run: nix-shell --run "uv run make -C core ${{ env.ACTIONS_DO_UI_TEST == 'true' && 'test_emu_multicore' || 'test_emu' }}"
+        run: nix-shell --run "uv run make -C core ${{ env.ACTIONS_DO_UI_TEST == 'true' && 'test_emu_ui_multicore' || 'test_emu' }}"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/core.yml around lines 334 - 339, The N4W1 matrix branch
incorrectly uses test_emu_multicore instead of the UI-aware target, which drops
UI checks when ACTIONS_DO_UI_TEST is true; update the N4W1 run line to mirror
the non-N4W1 conditional expression so it selects test_emu_ui_multicore when
ACTIONS_DO_UI_TEST is 'true' (i.e., use the same ternary logic with
ACTIONS_DO_UI_TEST -> 'test_emu_ui_multicore' || 'test_emu' instead of switching
to 'test_emu_multicore'), ensuring the UI flags (--ui=test, --ui-check-missing,
--do-master-diff) are preserved for N4W1 runs.

core/src/trezor/ui/layouts/eckhart/recovery.py (1)

224-236: ⚠️ Potential issue | 🟠 Major

Handle unexpected select_word() values explicitly.

This still relies on a 2-item tuple lookup, so any unexpected index turns into a generic IndexError. Given the UI contract here, it should fail deliberately and readably instead.

Proposed fix

         index = await interact(
             trezorui_api.select_word(
                 title=title,
                 description=description,
                 words=(TR.backup__type_n4w1, TR.backup__type_wordlist, ""),
             ),
             br_name="choose_method",
         )
-        return (BackupMethod.N4W1, BackupMethod.Display)[index]
+        if index == 0:
+            return BackupMethod.N4W1
+        if index == 1:
+            return BackupMethod.Display
+        raise ValueError(f"Unexpected select_word index: {index}")

Based on learnings, trezorui_api.select_word() called from choose_method will only return 0 or 1 on Eckhart because the empty third word slot is disabled at the UI level. Unexpected return values (index >= 2) should trigger an explicit failure (e.g., raise ValueError) rather than silently defaulting to BackupMethod.Display.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/trezor/ui/layouts/eckhart/recovery.py` around lines 224 - 236, The
choose_method function uses the index from trezorui_api.select_word to pick from
a two-item tuple, which will raise a generic IndexError for unexpected values;
change choose_method (the async function calling trezorui_api.select_word) to
validate the returned index explicitly: accept 0 -> BackupMethod.N4W1, 1 ->
BackupMethod.Display, and if index is any other value raise a clear ValueError
(or custom error) with a descriptive message referencing the unexpected index
and the select_word call so failures are deliberate and readable.

core/src/apps/management/recovery_device/layout.py (1)

283-290: ⚠️ Potential issue | 🟡 Minor

Suppress the debug warning for method=None here as well.

None is the intentional default path, so this branch still logs Unsupported backup method: None on ordinary recovery calls when USE_N4W1 is off.

Proposed fix

-        if method is not BackupMethod.Display and __debug__:
+        if method not in (None, BackupMethod.Display) and __debug__:
             from trezor import log
 
             log.warning(__name__, "Unsupported backup method: %s", method)

Based on learnings, method=None passed to choose_backup_method() is intentionally distinct from BackupMethod.Display. None is reserved for a future flow where the user is prompted to choose between supported backup methods interactively. It should NOT be treated as equivalent to BackupMethod.Display, and debug-mode unsupported-method warnings should be suppressed for None (as well as for BackupMethod.Display).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/apps/management/recovery_device/layout.py` around lines 283 - 290,
The debug warning in choose_handler currently fires for method=None; update the
condition so it only logs when method is neither None nor BackupMethod.Display
(i.e., skip logging for the intentional default None path as well as the Display
enum). Modify the branch that checks BackupMethod to require method is not None
and method is not BackupMethod.Display before calling log.warning (refer to
function choose_handler, symbol BackupMethod, __debug__, and log.warning).

.github/workflows/core.yml (1)

55-69: Consider adding a btconly N4W1 lane too.

All new N4W1 matrix rows are coins: universal, so CI still won’t build or exercise the btconly variant. If N4W1 is expected to ship on both firmware types, this leaves a blind spot for btconly-only regressions.

Based on learnings, when advertising the Capability.N4W1 capability, reviewers should ensure it is added for all firmware types (both Bitcoin-only and universal builds). Do not wrap f.capabilities.append(Capability.N4W1) with utils.BITCOIN_ONLY. The only acceptable conditional guard is utils.USE_N4W1.

Also applies to: 133-138, 306-311

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/core.yml around lines 55 - 69, The CI matrix is missing a
btconly N4W1 lane and some code conditionally guards N4W1 with
utils.BITCOIN_ONLY; add a matrix row mirroring the existing N4W1 universal entry
but with coins: btconly and n4w1: true (e.g., alongside the T3W1 n4w1 entry) so
btconly builds are exercised, and remove any conditional guards that append
Capability.N4W1 under utils.BITCOIN_ONLY — ensure Capability.N4W1 is only gated
by utils.USE_N4W1; apply the same change to the other similar blocks that add
N4W1 (the other matrix sections and any code paths that call
f.capabilities.append(Capability.N4W1)).

tests/input_flows.py (1)

2227-2242: Add an explicit assert for method validation to match InputFlowSlip39BasicBackup.

In InputFlowSlip39BasicBackup.input_flow_eckhart, there's an explicit assert self.method in (Display, N4W1). Here, the dict lookup implicitly validates valid methods, but an invalid method would raise a cryptic KeyError instead of a clear AssertionError.

♻️ Suggested fix for consistency

     def input_flow_eckhart(self) -> BRGeneratorType:
+        assert self.method in (
+            messages.BackupMethod.Display,
+            messages.BackupMethod.N4W1,
+        )
         num_screens = {
             messages.BackupMethod.Display: 10,
             messages.BackupMethod.N4W1: 8,
         }[self.method]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/input_flows.py` around lines 2227 - 2242, Add an explicit assertion to
validate self.method at the start of input_flow_eckhart so it mirrors
InputFlowSlip39BasicBackup: assert self.method in
(messages.BackupMethod.Display, messages.BackupMethod.N4W1). This makes method
validation explicit and raises an AssertionError with a clear failure mode
before using the dict lookup that currently raises a KeyError; place the assert
immediately before the num_screens assignment in the input_flow_eckhart method.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

core/src/apps/management/reset_device/layout.py (1)

237-246: ⚠️ Potential issue | 🟠 Major

Reuse the existing backup disclosure flow before N4W1 writes.

intro() is a no-op and backup() starts exporting shares immediately, so the N4W1 path skips the intro/safety acknowledgement that _DisplayBackup shows before exposing mnemonic material.

Possible stopgap

         async def intro(self, num_of_words: int | None = None) -> None:
-            # TODO: design/copy
-            pass
+            from trezor.ui.layouts.reset import show_intro_backup
+
+            await show_intro_backup(num_of_words=num_of_words)

         async def backup(self, iter_shares: Iterable[ShareInfo]) -> None:
-            # TODO: warn user about safety
+            from trezor.ui.layouts.reset import show_warning_backup
+
+            await show_warning_backup()

             # backup all shares
             for share in iter_shares:
                 await self._backup_share(share)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/apps/management/reset_device/layout.py` around lines 237 - 246, The
intro() method is a no-op and backup() immediately calls _backup_share, which
skips the existing _DisplayBackup disclosure/safety acknowledgement used in the
N4W1 flow; update intro() to present the same disclosure/acknowledgement flow
used by _DisplayBackup (or call the common method/component that shows the
copy/safety notice) and modify backup() to ensure it awaits/uses intro() before
iterating shares (i.e., call await self.intro(...) or invoke the shared
disclosure helper before calling self._backup_share for each ShareInfo) so the
safety intro is always shown prior to exposing mnemonic material.

core/src/apps/management/recovery_device/layout.py (1)

283-291: ⚠️ Potential issue | 🟡 Minor

Don't warn on the default method=None path.

In non-N4W1 builds this still logs Unsupported backup method: None for the ordinary recovery flow. Restrict the warning to explicit non-None unsupported values.

Proposed patch

-        if method is not BackupMethod.Display and __debug__:
+        if method not in (None, BackupMethod.Display) and __debug__:
             from trezor import log

             log.warning(__name__, "Unsupported backup method: %s", method)

Based on learnings, method=None is an intentional placeholder and should not trigger unsupported-method warnings.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/apps/management/recovery_device/layout.py` around lines 283 - 291,
The warning should not fire for the intentional placeholder value None; update
choose_handler so the log.warning inside the __debug__ block only runs when
method is not None and not BackupMethod.Display. Locate the choose_handler
function (and the log.warning call that references Unsupported backup method)
and change the condition to require method is not None in addition to method is
not BackupMethod.Display so only explicit unsupported enum values trigger the
warning; keep returning _DisplayHandler unchanged.

tests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.py (1)

38-39: Thread backup_method through the reset leg too.

The new fixture only affects the recovery half of this roundtrip. reset(session) still uses the default display backup flow, so the N4W1 matrix here never exercises reset-time backup.

Proposed patch

-def test_reset_recovery(client: Client, backup_method: BackupMethod):
+def test_reset_recovery(client: Client, backup_method: BackupMethod):
     session = client.get_seedless_session()

-    mnemonics = reset(session)
+    mnemonics = reset(session, method=backup_method)
     session = client.get_session()
     address_before = btc.get_address(session, "Bitcoin", parse_path("m/44h/0h/0h/0/0"))

-def reset(session: Session, strength: int = 128) -> list[str]:
+def reset(
+    session: Session,
+    strength: int = 128,
+    method: BackupMethod = BackupMethod.Display,
+) -> list[str]:
     with session.test_ctx as client:
-        IF = InputFlowSlip39BasicResetRecovery(session)
+        IF = InputFlowSlip39BasicResetRecovery(session, method)
         client.set_input_flow(IF.get())

         device.setup(
             session,
             strength=strength,
             passphrase_protection=False,
             pin_protection=False,
             label="test",
             backup_type=BackupType.Slip39_Basic,
+            backup_method=method,
             entropy_check_count=0,
             _get_entropy=MOCK_GET_ENTROPY,
         )

Also applies to: 52-52

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.py` around
lines 38 - 39, The test only applies the backup_method fixture to the recovery
half; thread the backup_method through the reset leg so reset uses the same
backup flow. Update the test_reset_recovery code paths that call
client.get_seedless_session() and reset(session) to pass the backup_method
fixture into the reset flow (for example by calling reset(session,
backup_method=backup_method) or by creating the session with
client.get_seedless_session(backup_method=backup_method) so both
get_seedless_session and reset() use the same backup_method); do the same change
for the other occurrence referenced (around line 52) so both reset and recovery
phases exercise the selected backup_method.